HBASE-26245 Store region server list in master local region #4136
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Ping @taklwu |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
apurtell
approved these changes
Mar 31, 2022
| * Mainly be used when restarting master, to load the previous active region server list. | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public interface RegionServerListStorage { |
There was a problem hiding this comment.
I think the 'Storage' part has a redundant meaning, let's just call this RegionServerList ?
| * Please see HBASE-26245 for more details. | ||
| */ | ||
| @InterfaceAudience.Private | ||
| public class MasterRegionRegionServerListStorage implements RegionServerListStorage { |
There was a problem hiding this comment.
Why not just MasterRegionServerList ? The 'Region' part of MasterRegion can be implied. See elsewhere that the 'Storage' part of RegionServerListStorage can also be implied.
| @@ -964,7 +969,8 @@ private void finishActiveMasterInitialization(MonitoredTask status) throws IOExc | |||
| this.regionServerTracker.upgrade( | |||
| procsByType.getOrDefault(ServerCrashProcedure.class, Collections.emptyList()).stream() | |||
| .map(p -> (ServerCrashProcedure) p).map(p -> p.getServerName()).collect(Collectors.toSet()), | |||
| walManager.getLiveServersFromWALDir(), walManager.getSplittingServersFromWALDir()); | |||
| Sets.union(rsListStorage.getAll(), walManager.getLiveServersFromWALDir()), | |||
There was a problem hiding this comment.
Do we still need to also track, and take a union here of, live servers via WALManager? Is the WALManager based live server stuff redundant now? Can we remove it?
There was a problem hiding this comment.
This is for rolling upgrading, where we have nothing in the master local region yet. I think we could remove the union in 4.0.0.
There was a problem hiding this comment.
There could be a migration step, and then taking the union is not necessary. The concern here is tracking by two mechanisms can cause issues or complicate troubleshooting if they get out of sync. It can be a follow up change if deemed important at some future time.
There was a problem hiding this comment.
Could be a follow up. It is not easy as the WAL directory is not actually a 'storage', so the 'migration' is not an actual migration as the WAL directory is always there even after 'migration'...
| import org.junit.experimental.categories.Category; | ||
|
|
||
| /** | ||
| * Simulate the scenario described in HBASE-26245, where we clean the WAL directory and try to start |
There was a problem hiding this comment.
This is excellent! Have been bitten by this scenario in test environments, nice to see a passing test for a solution.
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
added a commit
that referenced
this pull request
Mar 31, 2022
Signed-off-by: Andrew Purtell <[email protected]> (cherry picked from commit bb1bbdd) (cherry picked from commit 2711142)
Apache9
added a commit
that referenced
this pull request
Mar 31, 2022
Signed-off-by: Andrew Purtell <[email protected]> (cherry picked from commit bb1bbdd) (cherry picked from commit 2711142)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.